Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Remove reflection from FrameworkDescription #102164

Merged
merged 7 commits into from
May 15, 2024
Merged

Remove reflection from FrameworkDescription #102164

merged 7 commits into from
May 15, 2024

Conversation

am11
Copy link
Member

@am11 am11 commented May 13, 2024

Tiny help for trimming; shaved 2KB off of artifacts/bin/coreclr/osx.arm64.Release/System.Private.CoreLib.dll.

@dotnet-issue-labeler dotnet-issue-labeler bot added the needs-area-label An area label is needed to ensure this gets routed to the appropriate area owners label May 13, 2024
@dotnet-policy-service dotnet-policy-service bot added the community-contribution Indicates that the PR has been added by a community member label May 13, 2024
@am11 am11 added area-System.Runtime community-contribution Indicates that the PR has been added by a community member and removed community-contribution Indicates that the PR has been added by a community member needs-area-label An area label is needed to ensure this gets routed to the appropriate area owners labels May 13, 2024
@dotnet-policy-service Dotnet Policy Service
Copy link
Contributor

Tagging subscribers to this area: @dotnet/area-system-runtime
See info in area-owners.md if you want to be subscribed.

jkotas
jkotas reviewed May 13, 2024
View reviewed changes
src/libraries/System.Private.CoreLib/src/System/Environment.cs Outdated
@@ -201,21 +201,17 @@ public static OperatingSystem OSVersion
}
}

internal const string InternalVersion = "9.0.0";
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I do not think we want to add yet another place to update for version bumps. If you want to do this, it needs to be wired to eng/Versions.props.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The version update bump looks like this 96c2e1d once a year. Is it really a big deal to update a string? If you think it is, lets close this PR then. It was a simple improvement with no visible effect on production bits.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The version bumps are done every month in servicing, and they look like this: #101779

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, the patch version. 🤭

Let me think a bit how to dedup it.

jkotas
jkotas reviewed May 13, 2024
View reviewed changes
...untime/tests/System.Runtime.InteropServices.RuntimeInformation.Tests/DescriptionNameTests.cs Outdated
@@ -196,17 +196,8 @@ public void VerifyFrameworkDescriptionContainsCorrectVersion()
return;

Assert.DoesNotContain("+", version); // no git hash

#if STABILIZE_PACKAGE_VERSION
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this should stay as is. It is a regression test for a bug that slipped through the cracks some time ago.

@am11 am11 force-pushed the feature/versioning/cleanups branch from 4c08f94 to db1420d Compare May 14, 2024 01:48
@am11 am11 force-pushed the feature/versioning/cleanups branch 2 times, most recently from 80d8f1f to 791a6e7 Compare May 14, 2024 07:03
@am11 am11 force-pushed the feature/versioning/cleanups branch from 791a6e7 to c1ce415 Compare May 14, 2024 07:10
am11
am11 commented May 14, 2024
View reviewed changes
src/libraries/System.Private.CoreLib/gen/System.Private.CoreLib.Generators.csproj
@@ -5,6 +5,7 @@
</PropertyGroup>
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Side note, <TargetFramework>netstandard2.0</TargetFramework> this can probably use $(NetCoreAppCurrent) as it is only used by corelib building for current version.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There may be cases where the generator runs on .NET Framework if people open the project in Visual Studio.

@am11
Copy link
Member Author

am11 commented May 14, 2024

Tested with ./build.sh (all subsets), ./build.sh libs only, then mono only and clr only. It generates ProductVersionInfo.g.cs under artifacts/obj correctly; but at different locations, which we can consolidate with <CompilerGeneratedFilesOutputPath> prop if needed, e.g. at the root of obj/ next to:

$ ls artifacts/obj/*version*
artifacts/obj/_version.c	artifacts/obj/_version.h	artifacts/obj/runtime_version.h

@am11 @jkotas
Update src/libraries/System.Private.CoreLib/gen/ProductVersionInfoGen…
bd4f66e 
…erator.cs

Co-authored-by: Jan Kotas <jkotas@microsoft.com>
@jkotas
Copy link
Member

jkotas commented May 14, 2024

It generates ProductVersionInfo.g.cs under artifacts/obj correctly; but at different locations

That's expected. Different CoreLib builds should not be sharing files.

jkotas
jkotas approved these changes May 14, 2024
View reviewed changes
Copy link
Member

@jkotas jkotas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you!

@build-analysis build-analysis bot mentioned this pull request May 15, 2024
Open
This was referenced May 15, 2024
Closed
Closed
@jkotas jkotas merged commit 426edd0 into dotnet:main May 15, 2024
146 checks passed
stephentoub
stephentoub reviewed May 15, 2024
View reviewed changes
src/libraries/System.Private.CoreLib/gen/ProductVersionInfoGenerator.cs
{
context.RegisterPostInitializationOutput(ctx =>
{
string? informationalVersion = typeof(ProductVersionInfoGenerator).Assembly.GetCustomAttribute<AssemblyInformationalVersionAttribute>()?.InformationalVersion;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure how our build system handles this. Is this guaranteed to always match what typeof(object).Assembly would in the built corelib?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As far as I can tell, the build system stamps the exact same AssemblyInformationalVersionAttribute on all binaries.

Ruihan-Yin pushed a commit to Ruihan-Yin/runtime that referenced this pull request May 30, 2024
@am11 @jkotas @Ruihan-Yin
Remove reflection from FrameworkDescription (dotnet#102164)
c100549 
* Remove reflection from FrameworkDescription

* Switch to source gen

* Address CR feedback

* Update src/libraries/System.Private.CoreLib/gen/ProductVersionInfoGenerator.cs

Co-authored-by: Jan Kotas <jkotas@microsoft.com>

---------

Co-authored-by: Jan Kotas <jkotas@microsoft.com>
@github-actions github-actions bot locked and limited conversation to collaborators Jun 15, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@jkotas jkotas jkotas approved these changes

@stephentoub stephentoub stephentoub left review comments

Assignees
No one assigned
Labels
area-System.Runtime community-contribution Indicates that the PR has been added by a community member
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@am11 @jkotas @stephentoub